Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

Gold Standard: spark-only version for creating and comparing golden files #361

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

apoorvedave1
Copy link
Contributor

What is the context for this pull request?

Gold standard for non-hyperspace plan stability suite

Parent Issue: #334

What changes were proposed in this pull request?

Gold standard for non-hyperspace plan stability suite (vanilla spark version)

Does this PR introduce any user-facing change?

no

How was this patch tested?

unit tests

@apoorvedave1
Copy link
Contributor Author

note to reviewers: There is one issue currently with this pr: when running an individual query against approved files, the tests fail. When running all queries at the same time, the tests pass. I haven't yet fixed this issue.

Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apoorvedave1 Could you create a PR with just *.scala and one query set?

@apoorvedave1
Copy link
Contributor Author

@apoorvedave1 Could you create a PR with just *.scala and one query set?

Thanks @imback82 , working on #377 for the suggestion

@imback82
Copy link
Contributor

Thanks!

Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update PR description with what each directory contains and where you got them from?

  • src/test/resources/tpcds-plan-stability/approved-plans-v1_4/ (also, simplified.txt vs. explain.txt)
  • src/test/resources/tpcds/

Also, update it with how you generated them (mostly you can copy the instruction from PlanStabilitySuite.scala?

import org.apache.spark.sql.execution.command.ExplainCommand
import org.apache.spark.sql.execution.exchange.{Exchange, ReusedExchangeExec}

// scalastyle:off filelinelengthchecker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is taken from OSS Spark, can you have a link to credit it correctly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you add guided comments if some portions of this file are modified for Hyperspace to accelerate the review process? (I don't need to review that's already from OSS Spark)


import com.microsoft.hyperspace.SparkInvolvedSuite

trait TPCDSBase extends SparkFunSuite with SparkInvolvedSuite {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for this file (if applicable)

"q12", "q13", "q14a", "q14b", "q15", "q16", "q17", "q18", "q19", "q20",
"q21", "q22", "q23a", "q23b", "q24a", "q24b", "q25", "q26", "q27", "q28", "q29", "q30",
"q31", "q32", "q33", "q34", "q35", "q36", "q37", "q38", "q39a", "q39b", "q40",
"q41", "q42", "q43", "q44", "q45", "q46", "q47", "q48", "q49", "q50",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to just take out q49 here? If this is possible, you can add a TODO and link to an issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants